DEV-9254 remove dataclassY#225
Conversation
|
Task linked: DEV-9254 Remove dataclassy from Python SDK |
f53b427 to
f331425
Compare
chestm007
left a comment
There was a problem hiding this comment.
I've had a quick look - looks good from what ive got through. Github is a flaming pile of... yeah... and is dying trying to render this PR. theres a few nit pick formatting issues, ie newlines, missing line at EOF, and all not being at the top of the file, all pretty minor, but need to be fixed. I'll pull locally and review on anything that isnt github.
|
Converted to draft while formatting fixes are put in place |
3bc224d to
b996101
Compare
|
Reset to Max's formatting changes. Allegedly inline with all of the required standards |
|
Converted to draft until requested changes are fixed |
a8b3c2d to
62b1d78
Compare
Signed-off-by: Boris Filin <boris.filin@zepben.com>
064460c to
b2f9274
Compare
| # file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
|
|
||
| from .dataclass_base import DataclassBase as DataclassBase, zb_dataclass as zb_dataclass |
There was a problem hiding this comment.
question: Any reason we are importing these into this package? We generally don't import into the containing package, so is this one special somehow, or is this a future route we are taking?
There was a problem hiding this comment.
I used the setup in dataclassY as an example. Their code is
# __init__.py
from .decorator import dataclass, make_dataclass
from .dataclass import DataClass, Internal
from .functions import fields, values, as_dict, as_tuple, replace
I guess I landed on the one example that is not representative... In that sense, it is an archaic, not futuristic approach.
With that said, I think for a library that is semantically standalone (not tightly tied to CIM by itself), it might make sense to have this sort of endpoint.
Your call - say the word and I'll change it.
There was a problem hiding this comment.
we normally have imports in the top level package, just not the sub packages. i.e. __init__.py in zepben.ewb
| @@ -12,8 +12,6 @@ | |||
| from zepben.ewb.model.cim.iec61970.base.wires.junction import Junction | |||
|
|
|||
| # noinspection PyArgumentList | |||
There was a problem hiding this comment.
nit: This should be removed also. I am pretty sure it will start applying incorrectly to things undeer it unintentionally.
|
|
||
| from datetime import datetime | ||
| from typing import Optional | ||
| from dataclasses import dataclass |
There was a problem hiding this comment.
nit: excess blank line after removal of old import
| from zepben.ewb.model.cim.iec61970.base.wires.power_electronics_connection import PowerElectronicsConnection | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
nit: extra blank line. There are quite a few of these across the files.
| # We are using class names instead of identities because dataclasses create their own class instances | ||
| assert expected == all_equipment_container_classes, "Should be checking all EquipmentContainer subclasses" | ||
|
|
||
| junction = Junction('j1') |
There was a problem hiding this comment.
question: Why did this mrid change?
| :return: A set of all concrete implementations of `cls` under `package` | ||
| """ | ||
| y = set() | ||
| y = {} |
There was a problem hiding this comment.
question: Why did this need to change? There is a lot more code here than before, and no description of why this change was needed.
| ## [1.4.0] - UNRELEASED | ||
| ### Breaking Changes | ||
| * None. | ||
| * CIM classes now have to be instantiated with keyword arguments. The only accepted positional argument is mrid (or equivalent identifier) |
There was a problem hiding this comment.
question: Are these the only breaking changes?
Were all object before hashed just by their mrid? If not, then this is a major/breaking change for adding objects to a hashable collection.

Description
We remove the dataclassy from the SDK, migrating to using the actual Python dataclass decorator. (Note: The library has not been physically removed yet as some of the downstream repos rely on it. But it is no longer used anywhere in the SDK). This is motivated by lack of ongoing support for dataclassy, as well as no-one on the team wanting to deal with its mess of magic code.
DataclassBasewas implemented to manually implement dataclass-like field initialisation.@zb_dataclass- a normal dataclass decorator with some of its defaults changed (egslots=True)Identifiableand are based on themridIdentifiablemetaclass=ABCMeta. This makes zero difference programmatically, but makes it clearer which part of the model you are in.The changes and reasoning are described a lot more verbosely in
src/zepben/ewb/dataclass_descriptors/MANIFESTO.md.Associated tasks
No prerequisites.
Note that the code in this PR heavily anticipates the incoming nullable list change. Specifically, the
BackedDescriptorclass is currently only used in one place, but will be integral soon. Also, custom inits look a bit bulky with*argsand**kwargsforwarding, but they will all be removed soon.Test Steps
Run the test suite with the new tests; Ideally load a real model to make sure no side-effeects slipped through the cracks.
Checklist
If any of these are not applicable, strikethrough the line
~like this~. Do not delete it!. Let the reviewer decide if you should have done it.Code
Security
When developing applications, use following guidelines for information security considerations:
* Access to applications should be protected with security keys/tokens or usernames and passwords;* All sessions are encrypted if possible;* All application input is sanitised before being acted on (ie SQL statements, etc);* Log messages, and especially client-facing ones, must be handled securely and must not leak credentials information (internal URLs, passwords, tokens).Documentation
Breaking Changes
The internals of classes have changed, potentially leading to unknown side-effects (tests all pass). More importantly, You can no longer use positional arguments except for mrid (MyClass("mrid", 42) bad, MyClass("mrid", thing=42) good).